884: Ruby 4 upgrade#13
Conversation
| when 400..499 | ||
| raise MobilizeAmericaClient::ClientError, "Client Error (#{response.status}): #{response.body}" | ||
| when 500..599 | ||
| raise MobilizeAmericaClient::ServerError, "Server Error (#{response.status}): #{response.body}" |
| @@ -32,12 +32,15 @@ def request(method:, path:, params: {}, body: {}) | |||
| req.body = ::JSON.generate(body) unless body.empty? | |||
There was a problem hiding this comment.
Tangential to this PR, but does this do the right thing if e.g. we get an HTML response (which can happen sometimes if we get e.g. a Cloudflare error page)? Is this different from the standard faraday json thing?
(I'm just curious, this in no way needs to block this PR.)
There was a problem hiding this comment.
I think it is useful for the clients to not have to handle anything other than JSON, but I do agree that the error handling here could be better. (Don't think that is a job for this PR though.)
AFAIK Faraday's JSON Middleware internally uses json from Ruby's Standard Library, so this shouldn't change how it previously worked.
There was a problem hiding this comment.
I will change this to let Faraday handle the JSON parsing (for req and res). I assume they have a neat way of handling the HTML responses
| `git ls-files -z`.split("\x0").reject { |f| f.match(%r{^(test|spec|features)/}) } | ||
| end | ||
| spec.bindir = 'bin' | ||
| spec.executables = spec.files.grep(%r{^bin/}) { |f| File.basename(f) } |
There was a problem hiding this comment.
I know this was in here before, but is the stuff in bin actually useful to clients of the gem? Or just developers? (Or, uh, no one at all, if they're just automatically generated stubs)
There was a problem hiding this comment.
Autogenerated stubs - best to leave it in IMO
There was a problem hiding this comment.
🤷 I'm happy to keep the stubs, but making them part of the released gem feels like an accident. It would be unexpected for a client who installed this gem in their project to type "console" or "setup" and get something specific to the gem.
(My understanding is that the executables setting is intended for gems like rake, where the point of installing it is that now you can run rake in your project.)
There was a problem hiding this comment.
I havent released the gem yet so I can take it out
2 participants
No description provided.